Update KAMT to update the cached root on set_root#1667
Merged
Conversation
588be3d to
5fd2078
Compare
rvagg
approved these changes
Apr 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix KAMT set_root CID caching
Issue
This PR fixes an issue reported in recallnet/contracts#98 where the Filecoin EVM was incorrectly handling contract state after a delegate call from a constructor.
Root Cause
The KAMT data structure, which Filecoin's EVM uses to store contract storage slots, has a performance optimization that caches the last flushed root CID. This prevents unnecessary recomputation when no changes occur.
In
fvm_ipld_kamt@v0.4.0, a bug was introduced in theKamt::set_root()method: when setting a new root, it correctly updated the root node but failed to update the cached root CID. This meant that subsequentflush()calls without intermediate modifications (which would clear the cached root) would return the old cached CID instead of the new root CID.Reproduction Scenario
The bug manifests in this sequence:
cid_a.kamt.set_root(cid_b).kamt.flush()without modifying any storage slots.Expected:
flush()returnscid_b.Actual:
flush()incorrectly returnscid_a.EVM Impact
This bug mostly affects delegate calls within contract constructors:
contract_state.set_root()is called to update state, but the cached CID isn't updated.contract_state.Most contracts don't trigger this issue because usually the only way to modify the contract's state is to update a storage slot. However, during construction, the last operation is to install the contract's bytecode and flush the contract state (including that new bytecode) whether or not the contract's storage slots have been modified.
There are a few other ways to trigger this bug including re-entrency in a self-destructed contract and, e.g., some forms of re-entrency when constructing new contracts with the
CREATEinstruction (updating the contract's nonce will cause the state to be flushed).Fix
The fix is simple: update
set_root()to also setself.flushed_cid = Some(*cid)when loading a new root. This ensures the cached CID correctly reflects the current root.